-
-
Notifications
You must be signed in to change notification settings - Fork 238
fix(trigger): avoid render-based reset for interaction-level deduplication #601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
流程总结在 更改清单
预估代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 可能相关的 PR
建议审查者
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @QDyanbing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在修复触发器组件中的一个关键错误,该错误导致在受控模式下,事件去重状态无法正确重置,从而可能阻止 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
这个 Pull Request 旨在修复一个与事件去重逻辑相关的问题。原先,用于交互级别去重的 lastTriggerRef 的重置依赖于组件的 re-render,这在受控模式下如果外部没有同步更新 open 状态,可能导致组件不 re-render,从而使去重状态在多次用户交互之间残留,引发后续 onOpenChange 事件被错误地忽略。修复方案将 lastTriggerRef 的清理时机从 render 周期中移除,改为在当前用户交互批次结束时通过 queueMicrotask 进行清理。这个改动是正确且有效的,从根本上解决了问题,确保了在各种场景下事件处理的正确性。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.tsx
🔇 Additional comments (1)
src/index.tsx (1)
375-375: 变更看起来不错!新增的
resetScheduledRef用于防止重复调度清理任务,这个设计是合理的。
| const internalTriggerOpen = useEvent((nextOpen: boolean) => { | ||
| // `lastTriggerRef` is for interaction-level deduplication; do not reset it on render. | ||
| if (!resetScheduledRef.current) { | ||
| resetScheduledRef.current = true; | ||
| queueMicrotask(() => { | ||
| resetScheduledRef.current = false; | ||
| lastTriggerRef.current = []; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the code context around lines 377-385
wc -l src/index.tsxRepository: react-component/trigger
Length of output: 84
🏁 Script executed:
# Read the specific lines in context
sed -n '370,395p' src/index.tsxRepository: react-component/trigger
Length of output: 982
🏁 Script executed:
# Search for queueMicrotask usage in the file
rg -n "queueMicrotask" src/index.tsx -A 3 -B 3Repository: react-component/trigger
Length of output: 396
🌐 Web query:
queueMicrotask browser compatibility and support
💡 Result:
Short summary
- Native support: modern browsers and runtimes — Chrome 71+, Firefox 69+, Safari 12.1+, Edge (Chromium) 79+, Opera 58+, Node.js 11+. Also available in Web Workers. [1][2]
- Global usage (Can I use): ~95% of users covered; older browsers (IE, very old mobile/desktop versions) lack support. [2]
- Feature-detect: typeof queueMicrotask === 'function'. If missing, use a polyfill (core-js provides one) or fallback to Promise.resolve().then(...). [1][3]
Sources
- MDN Web Docs — queueMicrotask (Window / WorkerGlobalScope). [1]
- Can I use — queueMicrotask support table. [2]
- Polyfill note / core-js reference. [3]
🏁 Script executed:
# Check package.json for browser target configuration
cat package.json | head -50Repository: react-component/trigger
Length of output: 1356
🏁 Script executed:
# Search for any polyfill or compatibility handling for queueMicrotask
rg "queueMicrotask|Promise.*resolve.*then|polyfill" --type-list | head -20Repository: react-component/trigger
Length of output: 456
🏁 Script executed:
# Check if there's any Babel configuration that might handle polyfills
ls -la | grep -E "babel|tsconfig|\.browserlistrc"Repository: react-component/trigger
Length of output: 130
🏁 Script executed:
# Search for browser targets in tsconfig or similar
find . -maxdepth 2 -name "tsconfig*.json" -o -name ".babelrc*" -o -name "browserslist"Repository: react-component/trigger
Length of output: 82
🏁 Script executed:
# Check tsconfig.json
cat tsconfig.jsonRepository: react-component/trigger
Length of output: 441
🏁 Script executed:
# Search for polyfill or queueMicrotask usage more broadly
rg "queueMicrotask" -B 3 -A 3Repository: react-component/trigger
Length of output: 466
🏁 Script executed:
# Check if there's any polyfill setup
rg "polyfill|Promise.*then|core-js" --type ts --type tsxRepository: react-component/trigger
Length of output: 94
🏁 Script executed:
# Look for any global setup or index files that might handle compatibility
cat src/index.tsx | head -100Repository: react-component/trigger
Length of output: 3214
去重逻辑正确,出色地解决了受控模式下的问题!
使用 queueMicrotask 是解决交互级别去重的理想方案:
- ✅ 确保同一交互批次内的多次事件(如
mouseenter+pointerenter)正确共享去重状态 - ✅ 保证不同用户交互之间的去重状态被及时清理,避免误判
- ✅ 不依赖组件重渲染,完美解决受控模式下的去重问题
微任务在当前同步代码完成后、下一次事件循环前执行,这正好符合交互级别去重的时序需求。
浏览器兼容性注意: queueMicrotask 在现代浏览器中支持良好(Chrome 71+、Firefox 69+、Safari 12.1+、Edge 79+),覆盖约 95% 的用户。若需支持更旧的浏览器,需添加 polyfill(可使用 core-js 或 Promise.resolve().then() 作为备选)。当前项目 tsconfig 的 target 为 esnext,说明已假定目标用户使用现代浏览器。
🤖 Prompt for AI Agents
In src/index.tsx around lines 377-385, the current use of queueMicrotask may
break in older browsers; replace direct queueMicrotask calls with a small
cross-platform microtask helper (e.g., const scheduleMicrotask = (fn: () =>
void) => (typeof queueMicrotask === 'function' ? queueMicrotask(fn) :
Promise.resolve().then(fn))); then use scheduleMicrotask(() => {
resetScheduledRef.current = false; lastTriggerRef.current = []; }); and add the
minimal TypeScript typing for the helper so the code compiles without changing
target or adding large polyfills.
问题说明
lastTriggerRef用于对同一次用户交互(pointer / mouse / focus 等)进行事件级去重,但其回收依赖组件 re-render。
在受控模式下,若外部未同步更新
open状态,组件可能不会 re-render,导致去重状态跨用户交互残留,后续
onOpenChange被错误吞掉。修复方案
调整
lastTriggerRef的回收时机,不再依赖 render,而是在当前用户交互批次结束时通过
queueMicrotask进行清理。Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.